-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client: fix fuse client hang because its pipe to mds is not ok #24172
Conversation
a99b083
to
c913e11
Compare
src/mds/Server.cc
Outdated
@@ -1777,6 +1777,11 @@ void Server::handle_client_request(const MClientRequest::const_ref &req) | |||
session->is_closing() || | |||
session->is_killing()) { | |||
dout(5) << "session closed|closing|killing, dropping" << dendl; | |||
if (session->is_closed()) { | |||
dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl; | |||
mds->send_message_client(new MClientSession(CEPH_SESSION_CLOSE), session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use MClientSession::create()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx
c913e11
to
15c0f7c
Compare
jenkins, retest this please |
src/msg/simple/SimpleMessenger.cc
Outdated
@@ -708,7 +708,7 @@ void SimpleMessenger::mark_down(Connection *con) | |||
lock.Lock(); | |||
Pipe *p = static_cast<Pipe *>(static_cast<PipeConnection*>(con)->get_pipe()); | |||
if (p) { | |||
ldout(cct,1) << "mark_down " << con << " -- " << p << dendl; | |||
ldout(cct,0) << "mark_down " << con << " -- " << p << dendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo this change
src/mds/Server.cc
Outdated
@@ -1777,6 +1777,11 @@ void Server::handle_client_request(const MClientRequest::const_ref &req) | |||
session->is_closing() || | |||
session->is_killing()) { | |||
dout(5) << "session closed|closing|killing, dropping" << dendl; | |||
if (session->is_closed()) { | |||
dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation is wrong here
src/client/Client.cc
Outdated
@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m) | |||
} else if (mdsmap->get_addrs(mds) != session->addrs) { | |||
session->con->mark_down(); | |||
session->addrs = mdsmap->get_addrs(mds); | |||
if (mds_sessions.count(p->first)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the mds
variable instead of p->first
.
src/client/Client.cc
Outdated
@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m) | |||
} else if (mdsmap->get_addrs(mds) != session->addrs) { | |||
session->con->mark_down(); | |||
session->addrs = mdsmap->get_addrs(mds); | |||
if (mds_sessions.count(p->first)) { | |||
ldout(cct, 1) << "handle_mds_map update connection " << dendl; | |||
session->con = messenger->connect_to_mds(session->addrs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right approach. We don't necessarily want clients reconnecting to the MDS before it reaches STATE_RECONNECT
.
I think a better option would be for the client to set the session to stale in the else if (newstate >= MDSMap::STATE_ACTIVE)
block below IF the session is down (because we marked it down when the addresses changed) AND conf->client_reconnect_stale
. Same logic as in Client::ms_handle_remote_reset
.
IOW, if the client knows that its session is stale, it shouldn't try to blindly reconnect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok. The root cause is we only marked down Pipe when the addresses changed,the Session is still STATE_OPEN. So we need update connection so that the session can be closed in later logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok.
We can detect this situation though. Try this:
diff --git a/src/client/Client.cc b/src/client/Client.cc
index 82577eff64f..98608349572 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -2663,9 +2663,7 @@ void Client::handle_mds_map(MMDSMap* m)
int oldstate = oldmap->get_state(mds);
int newstate = mdsmap->get_state(mds);
- if (!mdsmap->is_up(mds)) {
- session->con->mark_down();
- } else if (mdsmap->get_addrs(mds) != session->addrs) {
+ if (!mdsmap->is_up(mds) || mdsmap->get_addrs(mds) != session->addrs) {
session->con->mark_down();
session->addrs = mdsmap->get_addrs(mds);
// When new MDS starts to take over, notify kernel to trim unused entries
@@ -2674,10 +2672,21 @@ void Client::handle_mds_map(MMDSMap* m)
trim_cache_for_reconnect(session);
} else if (oldstate == newstate)
continue; // no change
-
+
+ /* If we get here, the MDS has changed state or (!) another MDS has taken
+ * over. (Keep in mind that it's possible (oldstate == newstate ==
+ * STATE_ACTIVE) but the addr has changed! This happens when the client
+ * misses epochs.
+ */
+
session->mds_state = newstate;
- if (newstate == MDSMap::STATE_RECONNECT) {
+ if (newstate >= MDSMap::STATE_RECONNECT) {
session->con = messenger->connect_to_mds(session->addrs);
+ /* If we missed reconnect, the MDS will reset the session and we'll only
+ * reconnect if client_reconnect_stale==true.
+ */
+ }
+ if (newstate == MDSMap::STATE_RECONNECT) {
send_reconnect(session);
} else if (newstate >= MDSMap::STATE_ACTIVE) {
if (oldstate < MDSMap::STATE_ACTIVE) {
15c0f7c
to
34065fa
Compare
src/client/Client.cc
Outdated
@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m) | |||
} else if (mdsmap->get_addrs(mds) != session->addrs) { | |||
session->con->mark_down(); | |||
session->addrs = mdsmap->get_addrs(mds); | |||
if (mds_sessions.count(p->first)) { | |||
ldout(cct, 1) << "handle_mds_map update connection " << dendl; | |||
session->con = messenger->connect_to_mds(session->addrs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok.
We can detect this situation though. Try this:
diff --git a/src/client/Client.cc b/src/client/Client.cc
index 82577eff64f..98608349572 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -2663,9 +2663,7 @@ void Client::handle_mds_map(MMDSMap* m)
int oldstate = oldmap->get_state(mds);
int newstate = mdsmap->get_state(mds);
- if (!mdsmap->is_up(mds)) {
- session->con->mark_down();
- } else if (mdsmap->get_addrs(mds) != session->addrs) {
+ if (!mdsmap->is_up(mds) || mdsmap->get_addrs(mds) != session->addrs) {
session->con->mark_down();
session->addrs = mdsmap->get_addrs(mds);
// When new MDS starts to take over, notify kernel to trim unused entries
@@ -2674,10 +2672,21 @@ void Client::handle_mds_map(MMDSMap* m)
trim_cache_for_reconnect(session);
} else if (oldstate == newstate)
continue; // no change
-
+
+ /* If we get here, the MDS has changed state or (!) another MDS has taken
+ * over. (Keep in mind that it's possible (oldstate == newstate ==
+ * STATE_ACTIVE) but the addr has changed! This happens when the client
+ * misses epochs.
+ */
+
session->mds_state = newstate;
- if (newstate == MDSMap::STATE_RECONNECT) {
+ if (newstate >= MDSMap::STATE_RECONNECT) {
session->con = messenger->connect_to_mds(session->addrs);
+ /* If we missed reconnect, the MDS will reset the session and we'll only
+ * reconnect if client_reconnect_stale==true.
+ */
+ }
+ if (newstate == MDSMap::STATE_RECONNECT) {
send_reconnect(session);
} else if (newstate >= MDSMap::STATE_ACTIVE) {
if (oldstate < MDSMap::STATE_ACTIVE) {
okay |
670f4e1
to
290504a
Compare
src/mds/Server.cc
Outdated
dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl; | ||
mds->send_message_client(MClientSession::create(CEPH_SESSION_CLOSE), session); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ukernel I think we tried this before but you said this should not be done. Comments?
If client missed some mdsmap, I think it's better to use mds incarnation to detect if there were mds failover. if there were failover, set oldstate to STATE_NULL |
src/client/Client.cc
Outdated
// in its dcache/icache. Hopefully, the kernel will release some unused | ||
// inodes before the new MDS enters reconnect state. | ||
trim_cache_for_reconnect(session); | ||
if (mdsmap->get_addrs(mds) != session->addrs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if mds it not up, mdsmap->get_addrs(mds) will cause crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will change it later.
43a4953
to
59897c7
Compare
how about following change. so that we avoid changing Server.cc diff --git a/src/client/Client.cc b/src/client/Client.cc
index ae65738975..91c33020fc 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -2726,7 +2726,10 @@ void Client::handle_mds_map(MMDSMap* m)
session->con = messenger->connect_to_mds(session->addrs);
send_reconnect(session);
} else if (newstate >= MDSMap::STATE_ACTIVE) {
- if (oldstate < MDSMap::STATE_ACTIVE) {
+ if (oldstate < MDSMap::STATE_RECONNECT) {
+ // missed reconnect
+ _closed_mds_session(session);
+ } else if (oldstate < MDSMap::STATE_ACTIVE) {
// kick new requests
kick_requests(session);
kick_flushing_caps(session); |
The oldstate and newstate both are not sure because client may lose any piece of Message. So if the newstate is in the middle of STATE_RECONNECT and STATE_ACTIVE the changes above wont't make any sense. What do you think of following modification?
|
It works. If the test is true. you need to avoid executing code blocks of "else if (newstate >= MDSMap::STATE_ACTIVE)" and "else if (newstate == MDSMap::STATE_NULL && mds >= mdsmap->get_max_mds())" |
Besides, it's better to close old session and let client to re-open session when mds is fully recovered |
59897c7
to
624cf8d
Compare
src/mds/MDSMap.h
Outdated
int get_incarnation(mds_rank_t m) const { | ||
std::map<mds_rank_t, mds_gid_t>::const_iterator u = up.find(m); | ||
if (u == up.end()) | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return MDS_GID_NONE
src/mds/MDSMap.h
Outdated
/** | ||
* Get MDS rank incarnation if the rank is up, else -1 | ||
*/ | ||
int get_incarnation(mds_rank_t m) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mds_gid_t get_incarnation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the return type of this method.
624cf8d
to
8334382
Compare
src/mds/MDSMap.h
Outdated
/** | ||
* Get MDS rank incarnation if the rank is up, else -1 | ||
*/ | ||
int get_incarnation(mds_rank_t m) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the return type of this method.
src/client/Client.cc
Outdated
@@ -2614,6 +2614,7 @@ void Client::handle_fs_map_user(MFSMapUser *m) | |||
|
|||
void Client::handle_mds_map(MMDSMap* m) | |||
{ | |||
int old_inc = 0, new_inc = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mds_gid_t
instead of int
b613610
to
44c11b9
Compare
If fuse client session had been killed by mds and the mds daemon restart or hot-standby switch happens right away but the client did not receive any message from monitor due to network or other whatever reason untill the mds become active again.Thus cause client didn't do closed_mds_session lead the seession still is STATE_OPEN but client can't send any message to mds because its pipe is not ok.So we should close the stale session so that it can be reopened again. Fixes: http://tracker.ceph.com/issues/36079 Signed-off-by: Guan yunfei <yunfei.guan@xtaotech.com>
* refs/pull/24172/head: client: fix fuse client hang because its pipe to mds is not ok Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
There is a risk client will hang if fuse client session had been killed by mds and
the mds daemon restart or hot-standby switch happens right away but the client
did not receive any message from monitor due to network or other whatever reason
untill the mds become active again.Thus cause client didn't do closed_mds_session
lead the seession still is STATE_OPEN but client can't send any message to
mds because its pipe is not ok.
So we should create pipe to mds guarantee any meta request can be sent to
server.When mds recevie the message will send a CLOSE_SESSION to client
becasue its session for this client is STATE_CLOSED.After the previous
steps the old session of client can be closed and new session and pipe
can be established and the mountpoint will be ok.
Fixes:http://tracker.ceph.com/issues/36079
Signed-off-by:Guan yunfei yunfei.guan@xtaotech.com